Conversation
The synthetic_id cookie was missing the HttpOnly attribute, allowing any XSS on a publisher page to exfiltrate the tracking identifier via document.cookie. HttpOnly is safe to set because integrations receive the synthetic ID via the x-synthetic-id response header and no client-side JS reads it from the cookie directly. Also documents the rationale for each security attribute (Secure, HttpOnly, SameSite=Lax, Max-Age) in the doc comment, and adds debug_assert guards against cookie metacharacter injection in both the synthetic_id value and cookie_domain. Closes #411
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-scoped security hardening. The HttpOnly addition is correct, the doc comment is excellent, and the change is minimal. One concern about the debug_assert! approach and a few smaller items below.
0 Blockers · 1 High · 3 Medium
There was a problem hiding this comment.
@prk-Jr sorry the previous review didn't have clear suggestions.
Solid, well-scoped security fix. The HttpOnly addition is correct and the doc comment is excellent. Three actionable findings — two about the sanitization approach, one about the assert!.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Approval
All findings from both review rounds have been addressed in the latest commit (2f3282f):
- Runtime safety:
debug_assert!and per-requestassert!replaced with allowlist sanitization +log::warn!on modification, and#[validate]onPublisher::cookie_domainfor fail-at-startup config validation. - Sanitization approach: Switched from denylist to allowlist (
is_ascii_alphanumeric+.-_), which is more robust against unexpected metacharacters. - Test coverage: Sanitization, passthrough, and validator tests all added.
- Documentation: Doc comments updated to reflect the new sanitization approach, with
# Examplesdoctest included.
The follow-up to move sanitization upstream (to avoid the header/cookie value mismatch window) is tracked separately.
Looks good — nice work.
2 participants
Summary
synthetic_idcookie was missingHttpOnly, allowing XSS on a publisher page to exfiltrate the tracking identifier viadocument.cookie.HttpOnlyis safe to add because no client-side JS reads this cookie — integrations receive the synthetic ID via thex-synthetic-idresponse header instead.debug_assert!guards against cookie metacharacter injection in both thesynthetic_idvalue andcookie_domain, and documents the rationale for each security attribute in the function's doc comment.Changes
crates/common/src/cookies.rsHttpOnlyto cookie format string; adddebug_assert!metacharacter guards; update doc comment to enumerate all security attributes with rationaleCloses
Closes #411
Test plan
cargo test --workspacecargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest run— pre-existing ESM/CJS failure onmain, unrelated to this changecd crates/js/lib && npm run formatcd docs && npm run formatcargo build --bin trusted-server-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)